Skip to content

MCP 2026-07-28 readiness (waves 0-6) + #571 binding fix + two-pass security review - #550

Merged
Weegy merged 137 commits into
mainfrom
feat/mcp-2026-07-28-wave0-wave1
Aug 7, 2026
Merged

MCP 2026-07-28 readiness (waves 0-6) + #571 binding fix + two-pass security review#550
Weegy merged 137 commits into
mainfrom
feat/mcp-2026-07-28-wave0-wave1

Conversation

@Weegy

@Weegy Weegy commented Jul 30, 2026

Copy link
Copy Markdown
Contributor

What

MCP 2026-07-28 readiness — waves 0–6 — plus the security review that followed.

Two things landed on this branch since it was opened:

The feature work (waves 0–6)

Area What
Public MCP endpoint POST /api/v1/mcp — stateless Streamable-HTTP, API-key authenticated, dark by default (PUBLIC_MCP_ENABLED=false mounts no router at all). Four default-deny gates: authentication → binding → per-tool allowlist → scope
OAuth RFC 9207 iss validation, explicit per-server delegation (per_user / service), single-flight refresh, CIMD as a third client-acquisition mode alongside manual and DCR
Privacy Fail-closed masking boundary on the public dispatch path — a result that cannot be shown to have crossed it is discarded, not returned
Task seam Generic long-running-task primitive with claim/lease, event tail, reaper; dev_job as first implementor; deferred sub-agent dispatch (off by default)
Idempotency At-most-once dispatch for write-capable tools, namespaced per calling principal
Transport Legacy HTTP+SSE marked deprecated per the 2026-07-28 revision, behind an operator toggle
Migrations 0031 (OAuth iss + delegation), 0032 (CIMD), 0033 (public MCP key bindings)

The review, and what it found

Two passes, deliberately from different vendors so a shared blind spot would show up as a disagreement rather than a silence.

Pass Found Fixed here Deferred
Claude + codex gpt-5.6-sol #1 16 (1 critical-class, 7 high) 14 3
codex gpt-5.6-sol #2 (audits the fixes) 8 (0 high) 6 2

The trajectory is the signal worth reading: 1 critical + 7 high on the first pass, zero high on the second.

The one that mattered most

mcpAuthDiscovery took an authorization-server metadata document's self-declared issuer without checking it against the URL the document came from (RFC 8414 §3.3). That value is not inert: it selects which stored client secret ensureClient loads, and tokenRequest then POSTs that secret to the token_endpoint from the same untrusted document.

A hostile MCP server could therefore advertise its own authorization server, claim issuer: https://login.company.example — an issuer the install already holds an enterprise client for — and collect that client secret at its own endpoint.

The existing discovery test asserted the vulnerable behaviour (advertised one AS URL, returned a different issuer, asserted success) and was green.

The rest of the fixes

Area Defect
SSRF The guard resolved DNS, then fetch resolved it again — two lookups, two answers. Now enforced at connect time through the repo's existing createGuardedAgent(), plus the literal-IP pre-check undici's lookup hook structurally cannot cover
Idempotency Cache key omitted the principal, so two API keys collided on a guessable key like invoice-42 — one tenant receiving another's cached result, and a pre-claim suppressing another tenant's write
Batching One HTTP request carrying thousands of JSON-RPC messages cost one rate-limit token and that many Postgres round-trips. Refused outright (batching was removed from MCP in 2025-06-18)
Concurrency A timeout released the slot while the work ran on, so the advertised ceiling bounded only un-timed-out calls
Error leakage SDK handler failures serialised error.message to the caller — Postgres relation names, hostnames, driver strings
Response caps Size limits applied after full buffering; token and registration responses had no cap at all
Body cap Measured re-serialised bytes, so a chunked body of megabytes of whitespace passed. Now measures wire bytes via the parser's verify hook
Audit Credentials in upstream error text persisted to mcp_call_log from two sinks, only one of which redacted
Deferred tasks createdBy was declared in the types and never threaded through — _list filtered on kind alone and _status took any UUID, so one caller could enumerate and poll another's tasks and results
Existence checks Reported revoked keys and disabled agents as healthy bindings

Deliberately NOT fixed here

Named rather than quietly carried, with a one-line reason each:

  1. MCP runtime transports use unguarded global fetch. Pre-existing — this PR's SSRF work covers the OAuth/discovery paths. A redirect to a link-local address is reachable. Also a stale comment at mcpRegistryClient.ts:407 claiming a guard exists. Wants its own PR.
  2. Never-settling work holds a concurrency slot permanently. The ceiling is now honest; making it recoverable needs AbortSignal threaded through every tool handler. Wants its own PR.
  3. Deferred sub-agent tools are registered into a process-global native registry without an agent qualifier, so two agents defining the same sub-agent name collide. A facet of the gap already recorded at buildOrchestrator.ts:373 ("a per-agent tool allowlist is a follow-up"). Off by default.
  4. Browser / channel / routine identity namespaces differ, so per-user MCP is unusable on channel and routine paths. Fails closed.
  5. OAuth refresh single-flight is process-local — concurrent refresh is possible across replicas.
  6. The global express.json parses up to 10 MB before any authentication runs. Application-wide; lowering it affects every route.
  7. mcp_call_log may retain PII (not credentials) from upstream error text. Masking it means running the privacy pipeline inside the audit writer.

Test plan

Every fix is mutation-verified: the guard is disabled, the suite must fail, the guard is restored. That caught three defects in the fixes themselves before they landed — a vacuous replay test, a masking exemption that could have smuggled an unmasked body past the assertion, and twice-invalidated harness strings.

Risk / blast radius

  • Highest-risk surface in the MCP cluster: an internet-facing route reaching the tool layer, including write tools. Dark by default; every gate is default-deny.
  • Three migrations. 0031's backfill is one-time and narrowed to operator tokens (it preserves existing behaviour rather than widening it); all three are idempotent and relation-anchored; no destructive DDL.
  • middleware gains a direct @modelcontextprotocol/sdk dependency it was already importing but never declared — without it a clean npm ci leaves the production import in publicMcpServer.ts unresolvable.

Review guidance

The 30k-line diff is roughly 11.4k source, 17k tests, 1k docs. The source that actually needs eyes is middleware/src/mcp/ (the endpoint and its gates), middleware/src/services/mcp* (OAuth), and the three migrations. The privacy boundary and the four authorization gates are where the risk lives.

Weegy added 30 commits July 30, 2026 09:14
MIGRATION_DOMAINS listed five domains and omitted middleware/migrations,
the core runtime domain holding 0001-0030. Every migration there had
shipped without ever being applied — or re-applied for the idempotency
check — against a real Postgres in CI: the entire MCP schema (0003, 0008,
0009, 0010/0013, 0012/0014, 0015/0016, 0017-0020) and every dev-platform
migration (0022-0030). Suspected during #330, now confirmed and closed.

No latent schema defect was exposed. All 30 files apply and re-apply
cleanly against pgvector/pgvector:pg16, in both possible domain orderings
and additionally with rows present. The domain is self-contained: no
cross-domain foreign keys, no object names shared with the other five
domains, and no extension dependency (gen_random_uuid is core since pg13).

The comment now records the three domains that remain uncovered, each of
which needs its own audit before being enabled.
No pg test touched MCP before this — only memoryStoreConformance,
pluginVerdictStore and skillLifecycleStore existed. Covers the registry
seed and catalog-kind backfill (0010 + 0013, including that 0013's UPDATE
actually lifts the official registry off the 'generic' column default),
the kind/auth_kind/source/registered_via CHECK sets, marketplace
provenance with ON DELETE SET NULL detaching an imported server from a
deleted catalog, the 0014 partial unique index on top-level MCP grants
(and that it leaves native grants alone), and the 0015/0016 OAuth surface
— authorize-time endpoint pinning plus token/flow cascade on server
delete. Each assertion was mutation-checked against a deliberately broken
schema.

A second suite covers what the CI gate structurally cannot: the CI
idempotency check re-applies against an EMPTY database, so it can never
catch a migration that only breaks once rows exist. It re-applies all 30
files with MCP rows in place, in its own throwaway database — re-running
0001/0003 drops and recreates the NOTIFY triggers, which must not happen
underneath a concurrently running suite.

Both suites skip when no test Postgres is reachable and scope every row
to a w04-mcp- tenant prefix. Pools are capped: the runner executes files
concurrently and ~16 other pg suites each hold a default-sized (max 10)
pool, so an uncapped extra pool here exhausts max_connections and cancels
an unrelated suite mid-run (observed on ConductorWebhookSubscriptionStore).
…capture outputSchema

Issue #547 (W1-3) — plumbing only, no canvas synthesis.

Discovery now keeps a tool's declared outputSchema: McpToolDescriptor and
McpDiscoveredTool gained an optional outputSchema, and listTools copies it
from tools/list (object-valued only; anything else is dropped). It rides
along in the existing mcp_servers.discovered_tools jsonb column, so it
survives a restart without re-discovery and needs no migration.
subAgentToolHydration rehydrates it on the way back out and seeds the
manager's cache, since mcpNativeHandler only closes over a tool name.

structuredContent is no longer discarded. A new extractStructured() reads
it and McpManager hands it to an optional McpManagerOptions.structuredSink
as { kind: 'structured_output', serverId, toolName, turnId, structured,
outputSchema? }. Error results and absent/null payloads emit nothing.

This is deliberately out-of-band rather than a widened return type.
callTool() still returns Promise<string> and NativeToolHandler is
untouched, which keeps the published plugin contract stable and keeps every
MCP result on the 'typeof result === string' path that gates Privacy Shield
masking in the orchestrator — a non-string result would bypass the shield.
The payload union is a discriminated 'kind' so #544 (MRTR) can add
'input_required' without another refactor.

renderToolResult is byte-for-byte unchanged and is now pinned by a golden
suite (text-only, mixed blocks, structuredContent-only, empty content,
array-valued structuredContent, isError, whitespace fallback, nullish). A
mutation check installs a hostile sink that rewrites and deep-mutates its
payload and returns a different object, then asserts the LLM-bound string
is unchanged; verified to fail against a deliberately in-band mutant.

Operator surface: a read-only 'returns structured output' badge in the MCP
Control Center, with en + de strings.
…ains-middleware

# Conflicts:
#	docs/CHANGELOG.md
…red-content-sidecar

# Conflicts:
#	docs/CHANGELOG.md
…le-flight refresh

Three live security/correctness defects in the MCP OAuth path.

D1 — no RFC 9207 `iss` validation. The OAuth callback trusted the `state`
parameter alone. `state` proves a response belongs to a flow we started; it does
NOT prove which authorization server issued the code, so a malicious or
compromised MCP server could steer the callback and have a code minted by one AS
redeemed at another. `iss` is now validated against the issuer bound to the flow
BEFORE the code is exchanged, so a rejected callback persists nothing — no token
row, no vault write. A mismatched `iss`, or an absent one from an AS that
advertised `authorization_response_iss_parameter_supported`, is rejected. That
advertisement is captured at authorize time in the new
`mcp_oauth_flows.iss_required` column rather than re-discovered at the callback,
for the same reason migration 0016 pinned the token endpoint: a server that can
flip the flag in between would simply opt itself out of the check.

D2 — silent 'operator' fallback (confused deputy). Both the operator router and
the runtime McpManager resolved the OAuth user key as `… ?? 'operator'`, so a
Teams or Telegram turn whose user had no mapped identity reached the customer's
MCP server holding the OPERATOR's token. Resolution now goes through the new
`services/mcpDelegation.ts` and the new `mcp_servers.delegation` column:
`per_user` yields no token when no identity resolves and the turn fails closed
through the existing `onAuthFailure` path with an explanation; `service` is the
explicit opt-in to one shared identity. The fallback literal is gone from every
call site.

D3 — refresh race. `getValidAccessToken` permitted N concurrent refreshes per
(server, user). Against an AS with rotating refresh tokens the losers get
`invalid_grant` and the last writer can persist an already-retired token,
silently disconnecting the user. Concurrent callers now share one in-flight
promise keyed by (serverId, userKey), cleared in a `finally` so a failed refresh
never poisons later attempts.

Also:
- `mcp_oauth_tokens.issuer` records which AS minted a token; a rotated issuer
  drops the stored token instead of replaying it against a different server.
- `mcp_call_log.acting_identity` records WHOSE authority each call used
  (`caller_agent` is the orchestrator slug, not the identity). Resolved via a
  new optional `McpAuthProvider.resolveIdentity`, threaded through `callTool`
  before the dispatch guard so denied calls are attributed too. An
  unattributable call is recorded as `unresolved`, never left blank.
- OAuth failure logging goes through `services/secretRedaction.ts`: tokens,
  `code`, and `code_verifier` can no longer reach a log line, including values a
  provider echoed back that we never minted. The callback's error page is
  redacted too.
- New `PUT /mcp-servers/:id/delegation` plus a delegation control in
  McpAuthSection, with `adminMcp.auth.delegation*` keys in en.json and de.json.

Tests: 40 in test/mcpOAuth.test.ts covering iss present/absent/mismatched/blank
and trailing-slash equivalence, fail-closed resolution, issuer rotation, and
redaction. The D3 test is mutation-checked — it asserts exactly ONE
token-endpoint HTTP request under 8 concurrent callers (verified to report 8 and
fail when the in-flight map is removed), not a count of mock invocations.

BEHAVIOUR CHANGE (operator-visible): a fail-closed `per_user` default for every
row would break installed deployments whose channel users reach MCP servers
today BECAUSE of the 'operator' fallback. Migration 0031 is therefore
deliberately asymmetric — every EXISTING `mcp_servers` row that already holds a
stored operator token is set to `delegation = 'service'`, preserving today's
behaviour, and only NEWLY created servers get the safe `per_user` default.
Operators must review grandfathered servers and switch the ones that should be
per-user.
…s-delegation

# Conflicts:
#	docs/CHANGELOG.md
One hung sub-agent used to pin the whole Promise.allSettled batch for the
rest of the turn: domainQueryTool awaits agent.ask() with no abort and no
timeout, and there was no per-tool deadline anywhere in the orchestrator.

dispatchTool now races an AbortSignal-backed deadline (default 120s,
OMADIA_TOOL_DISPATCH_TIMEOUT_MS, 0 disables) and returns a structured
Error: result on timeout. The abandoned dispatch is marked aborted, so a
late result is discarded before the first write into turn state (raw-result
capture, canvas sentinel, KG ingestion, privacy interning) and late
sub-agent events are dropped by an abort-guarded observer.
…rized

callTool passed no RequestOptions and silently inherited the SDK's 60s
default, so the real ceiling was undocumented and un-tunable. It now
passes an explicit { timeout, resetTimeoutOnProgress, maxTotalTimeout }
(env-tunable), where resetTimeoutOnProgress keeps long streaming calls
alive and maxTotalTimeout is the absolute ceiling.

looksTransient() also matched a bare -32001, contradicting its own
contract: the code is implementation-defined and servers legitimately use
it for Unauthorized (omadia's LoopbackMcpServer does), so a genuine auth
failure got one doomed retry before surfacing. Auth now wins; a real SDK
request timeout still retries via its "Request timed out" message.
The mutation check is captureRawToolResult: a real turn-state write the
routine runner reads back. Verified by temporarily removing the
deadlineSignal guard — the suite then fails on the capture assertion, not
on a missing error string. Also covers batch siblings resolving normally,
the 0-disables path, and a bad env value falling back to the default.
The re-apply-under-data suite originally built its private copy of the
domain in a throwaway database. That isolates correctly, but CREATE/DROP
DATABASE is a cluster-wide operation: run inside the full suite with a
test Postgres reachable, it stalled the concurrently executing
dev-platform pg suites long enough that 29 of their tests were cancelled
with "test did not finish before its parent". Reproduced deterministically
against appStore.pg.test.ts and devJobStore.pg.test.ts, and confirmed
absent from the same run with this file removed.

It now runs against a dedicated schema on one pinned connection with
`public` off the search_path. The migrations name every object
unqualified, so they build a private copy there and never touch — or take
ACCESS EXCLUSIVE on — the shared tables. Cancellations: 29 -> 0. The test
asserts the isolation itself (table count in the schema), because a
leaked search_path would turn the migrations into no-ops against the
shared tables and make every later assertion pass vacuously.

Both suites now share the file's single capped pool, closed once in a
file-level after hook.
…MCP server

W0-3 — sort the dynamic tool segments by name so the Anthropic prompt-cache
tool block is byte-stable across machines and deploys.

`buildToolsList()` stamps `cache_control: {type:'ephemeral'}` on the last tool
spec, which makes the whole tool block a single cacheable chunk. The cache keys
on a byte-exact prefix, but two of the segments feeding that block were iterated
straight out of Maps — plugin load order for the native tool registry,
`created_at` row order for domain tools — with no sort anywhere. Stable within
one process, divergent across Fly machines and across deploys: a silent,
signal-free cache miss for the tool block and everything after it.

- new `toolOrdering.ts`: `compareToolNames` (locale-pinned `localeCompare(b,'en')`
  so the result does not depend on the host's LANG/LC_COLLATE), `sortByToolName`,
  `sortBySpecName`, `normalizeDiscoveredToolOrder`
- `buildToolsList()`: native + domain segments sorted; the deliberate
  fixed-literal prefix (memory, knowledge-graph, ...) keeps its existing order
- `ToolDispatchService.listDispatchableToolSpecs()`: sorted, so the loopback
  server and the CLI bridge inherit it
- `LoopbackMcpServer` tools/list: sorted independently, because `deps.tools` is
  caller-supplied
- `resolveSubAgentTools()`: sorted (grants arrive in `created_at` order)
- `setMcpDiscoveredTools()`: normalizes by name before persisting, so a server
  that returns `tools/list` in a different order each call stops churning the
  JSONB column and any grant-epoch diff derived from it

Ordering is advertisement-only. Collision resolution is unchanged — native tools
still win a duplicate name, decided by Map insertion and never by array position
— and that is now pinned by a test whose colliding name deliberately sorts last.

W1-2 — make the loopback MCP server stateless.

`sessionIdGenerator: undefined` is the SDK's stateless mode: no session id is
issued and no session validation is performed, so the CLI bridge needs neither
the `initialize` handshake nor `Mcp-Session-Id`. The previous comment claiming
session ids "remain required by the protocol" was wrong.

SDK 1.29.0 enforces the other half of that contract — a stateless transport
throws "Stateless transport cannot be reused across requests" on its second use.
The MCP server + transport pair is therefore built per request (matching the
SDK's own stateless example) and torn down in a `finally`. Both are in-memory
handler tables with no I/O, and this server sees a handful of requests per CLI
turn. `enableJsonResponse` stays on, which also guarantees the response is fully
written before teardown.

Non-POST is now declined with 405, which the MCP spec explicitly allows for the
optional GET SSE stream. Without it the per-request transport leaks: a GET stream
never ends, so `handleRequest` never resolves and the request scope never tears
down. Under the old stateful transport a session-less GET was rejected with 400,
so nothing regresses.

Tests were written first for W1-2 and observed to fail (HTTP 400) before the
production change. The wire test is parameterized over replaying vs never
sending the session header, plus a case with no `initialize` at all. The 401
`-32001` body and the 413 oversized-POST case still hold.

Deliberately NOT implemented: the `ttlMs` tool-list cache also proposed in #545.
Its premise is false — `subAgentToolHydration` reads
`mcp_servers.discovered_tools` and never calls `listTools`, so steady state is
already ~4 wire calls per server per day — and it would re-advertise removed or
repurposed tools inside exactly the window the #454 scan-verdict gate exists to
close.

MANUAL VERIFICATION REQUIRED BEFORE MERGE: the loopback server's only consumer
is the Claude CLI bridge, spawned with `--strict-mcp-config --mcp-config <path>
--allowedTools mcp__omadia__*`. If the installed CLI refuses to proceed when the
server issues no `mcp-session-id`, the bridge yields a server with ZERO tools and
the turn silently degrades to a toolless answer rather than erroring — no
automated test catches that. A pass against the real `claude` CLI with the
stubbed `createLoopbackServer` bypassed is needed. Not performed here: spawning a
nested `claude` session is blocked in this environment. Installed CLI version on
this machine is 2.1.220.
Nothing in the repo proved the client could complete an initialize →
tools/list → tools/call sequence: mcpCallAudit dials a refused port,
mcpRescan stubs listTools, and the cliBridge tests stub the server. These
drive a live in-process LoopbackMcpServer, sometimes through a recording
proxy that injects one transport-level failure, so retry and pool
behaviour are observed rather than inferred:

- listTools + callTool succeed over the wire; a second call reuses the pool
- a successful call is audited as ok (previously uncovered)
- a genuine Unauthorized surfaces immediately: exactly one POST, exactly
  one pool invalidation (fails if -32001 is classified transient again)
- the shipped once-retry still fires exactly once for -32000 and then
  succeeds, dropping the pooled connection once
- a stale token invalidates the pool and the next call reconnects
…-28)

MCP 2026-07-28 reclassifies the legacy HTTP+SSE transport as Deprecated with a
minimum 12-month removal window. Discourage 'sse' for NEW registrations while
keeping every existing SSE server fully working — no protocol work,
SSEClientTransport stays wired and the DB CHECK is untouched.

- DEPRECATED_MCP_TRANSPORTS + isDeprecatedMcpTransport in mcpClient.ts as the
  single source of truth, re-exported from @omadia/orchestrator.
- mcpNode() gains an additive transportDeprecated flag derived from it (no
  migration); exported for unit tests.
- Marketplace import path (the second way an sse row can be minted): prefer an
  http remote when a catalog entry offers both, still allow an sse-only entry
  but flag it via McpCatalogEntry.transportDeprecated.
- web-ui McpServerNode gains optional transportDeprecated; McpTransport keeps
  'sse' (published plugin contract stays as-is).

Refs #541
…ggle

Issue #541 acceptance 4 + 6. http (Streamable HTTP) stays the default and the
only remote option shown; 'sse' appears in the picker only after ticking 'Show
deprecated transports', labelled '(deprecated)'. Existing sse rows get a
Deprecated badge in the transport column with a hint pointing at Streamable
HTTP as the migration target.

Nothing is hard-blocked: the MCP removal window is at least 12 months, so an
operator can still deliberately register a legacy SSE server.

i18n: adminMcp.servers.{transportDeprecated,transportDeprecatedHint,
showDeprecatedTransports,deprecatedOption} in both en.json and de.json.

Refs #541
…gression

- mcpRegistryClient: prefers http when a catalog entry offers both remotes,
  still imports an sse-only entry (flagged), and the preference cannot bypass
  the untrusted-remote guard; the pre-existing sse fixture now also asserts
  transportDeprecated.
- mcpNode: transportDeprecated true for sse, false for http/stdio.
- Regression: an sse config still yields a real SSEClientTransport (and http a
  StreamableHTTPClientTransport) — the unit discourages, it does not remove.

Refs #541
Issue #541 acceptance 8 + 9. First test file for the MCP Control Center page:
the picker offers only http/stdio by default, exposes 'sse (deprecated)' after
ticking 'Show deprecated transports' and lets it be selected (never
hard-blocked), resets to http when the toggle goes off, badges existing sse rows
and falls back to the local list when an older middleware omits the flag.

Refs #541
…n' into feat/mcp-2026-07-28-wave0-wave1

# Conflicts:
#	docs/CHANGELOG.md
…t' into feat/mcp-2026-07-28-wave0-wave1

# Conflicts:
#	docs/CHANGELOG.md
…-sidecar' into feat/mcp-2026-07-28-wave0-wave1

# Conflicts:
#	docs/CHANGELOG.md
#	middleware/packages/harness-orchestrator/src/mcp/mcpClient.ts
…er-and-stateless-loopback' into feat/mcp-2026-07-28-wave0-wave1
…cp-client-tests' into feat/mcp-2026-07-28-wave0-wave1
Lift the shape devJobOrchestratorTool.ts hand-rolled (start returns a
handle at once, work runs detached, a card streams into the turn) into a
reusable seam so any tool can be marked longRunning.

- taskTypes.ts: TaskDescriptor + TaskStore/TaskReadStore, with a status
  vocabulary (working | input_required | completed | failed) chosen to
  project mechanically onto MCP Tasks later.
- inMemoryTaskStore.ts: reference implementor of the claim/lease and
  terminal-transition semantics.
- longRunningTool.ts: defineLongRunningTool() -> the non-blocking
  <tool>_start / _status / _list triple + pending card buffer.
- taskReaper.ts: orphan sweep (abandoned live tasks, accumulated
  terminal tasks).
- subAgentTaskTool.ts: deferred sub-agent dispatch as second consumer.

No MCP Tasks protocol handlers: internal LocalSubAgent dispatches never
cross an MCP boundary, and SEP-2663's tasks/update is unshipped.
Client ID Metadata Documents (issue #546) become the third link of an explicit
acquisition chain: stored -> cimd -> dcr (deprecated, warns) -> manual.

Corrects the issue's premise: the OAuth 2.1 + PKCE stack already shipped in
epic #459 W9, so this is a delta on it. CIMD replaces Dynamic Client
Registration at MCP-native brokers only -- Entra ID and Okta do not support it
and keep using the existing manual path, which has no sunset. DCR is kept
working and merely warns.

- migration 0032: 'cimd' in the registered_via CHECK set + client_metadata_url
- GET /.well-known/omadia-mcp-client, allowlisted via the shared constant
- 501 (not 500) when FLOW_PUBLIC_BASE_URL is unset -- CIMD needs INBOUND https
  reachability, so a firewalled install degrades to manual instead of breaking
- SSRF guard reuses assertPublicHttpsUrl on the metadata probe
- describeAuth gains acquisitionMode / cimdSupported / cimdBlockedReason
Additive adapter (new file only): projects DevJobStore onto the generic
TaskStore seam - ten-value DevJobStatus down to the four-value
MCP-Tasks-shaped vocabulary, dev_job_events onto the seam's event tail,
claimNextQueued onto claimNextPending, and finalizeDevJob onto finish so
the brand-gated terminal choke point is preserved.

Zero edits to devJobStore.ts / devJobOrchestratorTool.ts and no
migration: dev_job_start still returns {"status":"job_started",...} so
the web-ui card parser and existing tests are untouched. Keeping this in
its own file also keeps the in-flight dev-platform plugin extraction
(PR #536/#538) to a file move rather than a conflict.

Documents the one intentional divergence: the seam's finish() is
lease-fenced, dev_job's finishTerminal deliberately is not (cancel routes
and the reaper finalize jobs they never claimed), so the adapter accepts
a matching lease OR no lease, and rejects a mismatched one.
Extend the lenient CallToolResult schema with resultType + inputRequests,
both readable off the shipped SDK 1.29.0. An input_required result parks
the call in a new PendingMcpInput store keyed on the
{userId, sessionId, correlationId} triple and returns a stable sentinel
to the model instead of a result: no retry attempt consumed, no failure
audit row. The MCP call audit gains a three-valued outcome so a parked
call is neither reported as a failure nor as a delivered result.

Rides W1-3's McpSidecarKind union, which was shaped for exactly this
second member.
…guard, migration 0032

- strategy chain: stored beats cimd beats dcr beats manual, each asserted via an
  OBSERVABLE consequence (which client_id reaches the provider, what was
  persisted) rather than a mock call count
- CIMD skipped when the AS does not advertise support, when no metadata URL is
  configured, and when the document is not inbound-reachable
- metadata endpoint shape, 501 without FLOW_PUBLIC_BASE_URL, and a direct
  assertion that served redirect_uris === McpOAuthService.redirectUri
- SSRF rejection of loopback / RFC1918 / link-local / non-https metadata URLs,
  including that no request leaves before the guard runs
- publicPaths asserted against the shared CIMD_METADATA_PATH constant
- migration 0032 pg test isolated in a dedicated tenant SCHEMA (never a scratch
  database: CREATE/DROP DATABASE is cluster-wide and cancels concurrent suites)
…cy invariants

31 tests across the reference store and the registration helper. Each
invariant was verified by deliberately breaking it, rebuilding, and
confirming a real assertion failure:

- M1 lease fence removed          -> claim+lease suite fails
- M2 reaper keyed on heartbeat only -> never-claimed-orphan test fails
- M3 task input copied onto a card  -> privacy invariant test fails
- M4 takePendingCards stops draining -> card test fails
- M5 terminal immutability guard removed -> zombie-worker test fails

M5 initially SURVIVED: finish() cleared the lease, so the lease check
alone carried terminal immutability and the guard was unreachable. Fixed
properly rather than by weakening the claim - reapOrphans now PRESERVES
claimedBy, which is the correct semantics (the reaper is not the owner,
and a zombie worker waking up after being reaped legitimately still holds
a matching lease). The guard is now the only thing rejecting it, and the
new zombie-worker test fails without it.
… statement

A pool-level 'SET search_path' binds only the one pooled client that served it,
so the next query lands on a different client and silently resolves against
public. Two assertions failed on the first real run because of it. The
search_path is now a connection option on a dedicated pool, with a separate
admin pool for CREATE/DROP SCHEMA.
Weegy added 13 commits August 7, 2026 09:09
`idempotencyCacheKey` was `(key, toolName)` and the store is process-wide,
shared by every public MCP dispatcher. Two API keys — different customers,
bound to different agents — collide on any guessable idempotency string.

Key B calls `create_invoice` with `invoice-42`; A used the same key
minutes earlier. B receives A's cached RESULT and B's write never executes.
The conflict branch is the same defect pointed the other way: pre-claiming
a predictable key with a different payload refuses the real caller's write,
so any key holder gets a denial primitive against another tenant.

The old comment argued cross-agent collision 'needs the same key AND the
same tool name — at which point deduping is the correct answer anyway'.
True for one tenant. Across principals, deduping is precisely the wrong
answer: it is a cache hit across a trust boundary.

`namespace` is now a REQUIRED parameter, not a defaulted one. A default
would let a future call site inherit the leak silently, which is how this
class of bug survives review; requiring it makes every caller state which
principal it is deduping for. `ToolDispatchService` passes
`caller.principal` — the API-key id on the public path — and `''` when
there is no caller, i.e. the in-process chat/loopback path, which is one
trusted principal and behaves exactly as before.

Three guards added: no cross-principal replay, no cross-principal
pre-claim, and — as the guard rail that keeps the first two from being
vacuous — same-principal deduping still executes at most once. Also pins
namespace/tool separator ambiguity alongside the existing key/tool one.
Mutation-checked: dropping namespace from the composition fails both
isolation cases and leaves the guard rail green.
Every per-request control here is charged once per HTTP request:
`requireApiKey`'s rate limiter takes ONE token, and `tools/list` never
touches the concurrency counter. A JSON-RPC batch is one HTTP request
carrying many messages, so a single sub-8-MB array of tens of thousands of
`tools/list` calls costs the caller one token and costs the server that
many `bindings.get` round-trips to Postgres. The write limiter still
covered writes; reads and listing were free.

Reachable, not theoretical: the SDK's transport delegates to
`WebStandardStreamableHTTPServerTransport`, which maps over `rawMessage`
whenever `Array.isArray`.

Refused outright rather than rate-limited per message. It is the smaller
fix and the spec-correct one — MCP removed JSON-RPC batching in the
2025-06-18 revision and this endpoint implements 2026-07-28 — where
per-message limiting would mean reaching back into a limiter that lives one
middleware upstream, for a shape the protocol no longer defines.

Placed before `requireApiKey` so an unauthenticated batch also costs
nothing. Three cases: a batch is refused as -32600, a 5000-message batch
sitting deliberately UNDER the byte cap is refused on shape rather than
size, and — as the guard rail — an ordinary single request still passes.
Mutation-checked: removing the guard fails both refusals and leaves the
guard rail green.
…handler errors

Two defects on the public endpoint, both in the same call path.

TIMEOUT RELEASES A SLOT WITHOUT STOPPING THE WORK. `withTimeout` is a
`Promise.race`, and losing a race does not cancel the loser — nothing in
ToolDispatchService accepts an AbortSignal. The slot was released in a
`finally` on the RACE, so every timeout reopened a slot while the
Odoo/M365/MCP/sub-agent call behind it stayed alive. A caller hammering a
tool that hangs on an upstream connection accumulated unbounded sockets,
promises and external work while the endpoint still advertised a ceiling of
`maxConcurrentCalls`. The release now rides on the WORK, so the ceiling
counts what is actually running. Cancellation would mean threading an
AbortSignal through every handler and belongs on its own.

HANDLER ERRORS REACHED THE CALLER VERBATIM. `handleHttp`'s catch answers a
bare 'Internal server error' and is unreachable for anything a request
handler throws: the SDK converts a handler rejection into a JSON-RPC error
built from `error.message` (`shared/protocol.js`) and then RESOLVES, so
`handleRequest` never rejects. A failing `bindings.get` therefore handed an
external, merely API-key-authenticated caller a Postgres diagnostic — the
relation name, the database host, a driver string. Both handlers now route
through `sanitized()`, which passes deliberate `McpError`s through
untouched and turns anything else into a generic error, logging the real
one server-side.

Tests: the slot is still held after a timeout and returns once the
abandoned work settles; a store failure leaks neither the relation name nor
the internal hostname, on `tools/call` and `tools/list`. The harness gains
a `bindingStore` override, since a FAILING store is not expressible through
`bindingRows`. Both mutation-checked.
…anonicalise the issuer

SIZE CAPS RAN AFTER FULL BUFFERING. `await res.text()` followed by a length
check is not a cap: by the time the check runs the whole body is resident.
A discovered authorization server can stream hundreds of megabytes inside
the request timeout and the process allocates all of it before deciding it
was too large. `mcpOAuthClient`'s token and registration responses had no
cap at all — and those are the two endpoints whose URLs come from an
untrusted document.

`readTextCapped` meters the stream and cancels the reader the moment the
cap is passed, so the socket is torn down rather than drained. It also
honours a declared oversized `content-length` as a fast path. Applied to
metadata discovery (256 KB), the CIMD probe (64 KB), and both OAuth client
POSTs (256 KB). A `Response` with no body stream — a hand-built one in a
test fake — falls back to the previous buffered read plus the same cap, so
no fixture changes and no real network path takes that branch.

CANONICAL ISSUER. `AuthServerMetadata.issuer` now carries the issuer we
FETCHED rather than the string the document echoed back. The RFC 8414 §3.3
guard has already proven the two are the same identifier, so the only
remaining difference is trailing slashes — and that spelling is what keys
`loadClient()` and the stored token rows, where a document answering
`https://as.example/` could miss a client stored under `https://as.example`.
Taking the canonical form also retires the question of how many trailing
slashes `sameIssuer` should tolerate for everything downstream.
…sertion

The public endpoint requires that masking actually RAN for the current
dispatch — a positive signal, because 'did not fail' cannot distinguish
success from never-ran. An idempotency replay cannot satisfy that by
observation: no handler runs, and the gate is built per request, so
`masked()` is false however well the cached body was masked when it was
produced.

So a retried write — the exact case idempotency exists for — was answered
with 'privacy masking did not run'. That is the worst available reply: the
write DID commit, the endpoint suppressed the duplicate correctly, and the
caller is told something that reads like a failure and learns nothing about
whether the mutation happened.

`ToolDispatchResult.replayed` now marks a cache hit and the assertion
exempts it. Narrow and safe: the body crossed this same boundary on the way
in, and since idempotency is namespaced by principal (previous commit) a
replay can only ever be returned to the principal that produced it. Digest
RESOLUTION does not carry over — those bindings belong to the earlier turn.

The test for this was vacuous on the first attempt and the mutation check
caught it: `realDispatcher` wired no `ToolIdempotencyStore`, so
`dispatchIdempotent` skipped the cache and the case was two ordinary
dispatches that both masked fine — green with the exemption removed. The
harness now takes `idempotency: true`, and the test counts REAL handler
executions as its oracle, so 'no replay happened' fails loudly instead of
passing quietly.
The 8 MB gate measured `JSON.stringify(req.body)`. JSON is mostly
insignificant whitespace, so that is not the size of anything that was
received: a chunked body carrying megabytes of spaces around `{}` declares
no `Content-Length`, re-serializes to two bytes, and passed both the
declared-size and re-serialized checks — while costing the full transfer to
read and parse.

The one place the real figure exists is `express.json`'s `verify` hook,
which sees the raw buffer before parsing. `http/rawBodySize.ts` records it
on the request and the gate now prefers it, keeping the re-serialized check
as the fallback for requests that never met a `verify`-wired parser (a
raw-body route, a hand-built test request) — a weaker check where the real
figure is unknown beats none.

NOT fixed here, and worth stating plainly: the global parser still reads and
allocates up to its own 10 MB limit before ANY route runs, authentication
included. Lowering that ceiling affects every route in the application and
is a separate decision, not something to slip into an MCP commit.

Also restores two mutation-harness entries that my earlier edit to the
masking assertion silently invalidated — the harness reported them
'SKIPPED (mutation no longer applies)' and the run fell from 39/39 to 37/39.
A mutation that stops matching is lost coverage that still looks like a pass,
so both are re-pointed at the new source, and the replay exemption added in
the previous commit gets its own entry.
…audited

`mcp_call_log.error` carries a remote MCP server's own protocol/transport
message. The orchestrator bounds it to 300 characters and the comment there
notes external error strings can carry upstream data — but truncation is not
redaction. A server that echoes `refresh_token=…`, an
`Authorization: Bearer …`, or a secret-shaped JSON field puts that
credential into an append-only table, and the operator audit API returns the
stored string verbatim.

Redacted in the `onToolCall` sink rather than at the write in
`mcpClient`: `secretRedaction` lives in this package and the sink is the
injected seam, so the alternative was a second copy of the patterns inside
`@omadia/orchestrator` — and two copies of a redaction rule is one that
drifts loose.

LIMIT, stated rather than papered over: this removes CREDENTIALS, not PII.
An upstream error quoting a customer name or address still lands in the
table. Masking that means running the privacy pipeline inside the audit
writer, which is a design decision, not a patch.

Coverage note: `redactSecrets` itself is covered by `mcpOAuth.test.ts`;
this commit is two lines of wiring in the boot path, which has no unit seam
short of booting the app.
`NewTaskInput.createdBy` was documented as 'who asked for it — for scoping
reads' and `TaskListFilter.createdBy` existed to filter on it. Neither was
ever passed. `_start` created tasks with no owner, `_list` filtered on
`kind` alone, and `_status` accepted any task id. Two callers of the same
deferred tool could therefore enumerate each other's tasks — terminal ones
included, which carry `result` and `error` — and then poll each id for its
full event and result stream.

An injection point declared but never threaded through to its consumer,
which is the same shape as a long line of prior defects in this codebase.

`TaskDescriptor` now exposes `createdBy` so a READ can check it: `_status`
takes an id and nothing else, and the alternatives were to trust the id or to
re-list and hope the task fell inside the page limit. `_status` answers the
identical 'not found' string for 'no such task' and 'not yours', because
distinguishing them makes the handler an existence oracle over other callers'
task ids.

Owner comes from `currentDispatchCaller().principal` (the API-key id on the
public MCP path) or `turnContext.mcpUserKey` (chat and channel turns). Both
are server-attested, so neither can be spoofed. When neither resolves the
owner is the literal `'unidentified'` rather than `undefined` — an absent
value would DROP the optional filter and list everything, reintroducing the
leak on the one path that has no identity. Grouping unidentified callers is
the honest floor, not a guarantee. `createdBy: null` stays unscoped, which
is what dev_job wants: operator-initiated work behind an authenticated admin
surface.

Reachable only with `LONG_RUNNING_SUBAGENT_TOOLS` set; it defaults to empty.

Both halves mutation-checked separately, since they are enforced by different
mechanisms — `ownsTask` for the poll, the `createdBy` filter for the list —
and a single mutation only exercises one. GOTCHA worth recording: these tests
import from `@omadia/orchestrator`, i.e. the BUILT package, so a mutation in
`src/` proves nothing until the package is rebuilt. The first attempt at both
checks passed for exactly that reason.
…lementor names

CARD LEAK. Every `_start` pushed a card onto `pendingCards`, and
`takePendingCards()` — the only drain — has no production consumer on the
deferred sub-agent path: `subAgentToolHydration` keeps
`handle.registrations` and discards the handle. So the array grew for the
life of the process, one entry per call. The task reaper cannot help; it
clears task rows, not this closure's array. Capped at 100, dropping the
OLDEST, since surfacing these cards is unshipped work and the newest is what
a consumer would want if one ever attaches.

DECOUPLING RATCHET. The previous commit's explanatory comments named the
seam's first implementor three times inside `@omadia/orchestrator`, plus
once more in core, taking the epic #470 ratchet from 3448 to 3452 and
failing CI. Both halves of that are wrong, not just the count: this PR
already contains a commit titled 'describe the task seam by contract, not by
its first implementor', and my comments walked that back. Reworded to
describe the CONTRACT — 'an implementor that scopes no reads', 'implementors
that ship their own card consumer' — which is both what the ratchet wants
and what the seam's own design decision says. Held at 3448, no baseline
raise.

Worth recording: the ratchet counts TEXTUAL references, comments included.
A comment that names the thing being extracted moves the number the same as
an import does.
…y failure

Three defects from the second cross-vendor pass. The first is a hole I
introduced with the replay exemption.

REPLAY COULD SMUGGLE AN UNMASKED BODY PAST THE ASSERTION. The idempotency
store retained a dispatch result BEFORE the endpoint asserted that masking
had run, so the ordering was: unmasked body cached → first request correctly
refused → retry replays that cached RAW body, flagged `replayed` and
therefore exempt from the very assertion that had just rejected it. My
comment claiming the cached body 'crossed this same boundary on the way in'
was simply false. Reachable through a dispatcher that omits `withPrivacy`
(which the interface permits) or any future masking regression — precisely
what a fail-closed control exists to contain.

Fixed by asking the question at the right time: `ToolDispatchOptions.validateResult`
runs the caller's admissibility check INSIDE the store's `exec`, so a
refused body throws before retention and the store keeps no rejected
outcome. That also covers the concurrent duplicate, which collapses onto the
same execution and would have been handed the poisoned body before any
after-the-fact invalidation could run. The post-dispatch check stays as
defence in depth for the replay path and for dispatchers that ignore the
option.

DECLARED-OVERSIZE RESPONSES WERE NOT CANCELLED. `readTextCapped` returned
`null` on an oversized `Content-Length` without cancelling the body, and
the caller then cleared its abort timer — so a server answering
`Content-Length: 100000000` and trickling forever pinned a socket per probe.
Cancel first, then return.

FAILURES WENT UNAUDITED. The catch treated 'is an McpError' as 'already
audited'. Wrong in both directions: the TIMEOUT is an McpError minted inside
`withTimeout`, which records nothing — so a write that may have committed
upstream produced no row at all — and the masking refusal, now thrown from
inside `dispatch`, no longer passed the record that used to sit beside the
check. Replaced with an explicit written-once flag, so every failure lands
exactly one row and no path lands two.

Tests: an unmasked result refused on the first call must also be refused on
retry rather than served from cache; a timed-out call produces an audit row;
a masking refusal produces exactly one. All three mutation-checked —
restoring the `McpError` assumption fails the timeout case.
…udit sinks

Two halves of the same gap, from the second cross-vendor pass.

ONE SINK HAD NO REDACTION. `mcp_call_log` is written from two places — the
runtime observer and the Agent Builder sandbox observer — and only the
runtime one redacted. A sandbox test-call persisted the credential verbatim
into the same table. Both now go through `redactAuditError`, a shared
structural helper rather than a copy-pasted expression at each call site,
because a copy-pasted transform is precisely how one sink ended up redacting
and the other not.

THE PATTERNS ONLY KNEW OAUTH. This redactor was written for OAuth error
bodies, then picked up a caller that stores error text from ARBITRARY
upstream MCP servers. Those are not OAuth providers and do not use RFC 6749
spelling, so `X-API-Key: sk_live_…`, `Authorization: Basic …`,
`accessToken`, `clientSecret`, `api_key` and `password` all matched
nothing and were persisted in full. Added camelCase and generic credential
field names, a header-value rule for `X-API-Key`-style headers, and `Basic`
alongside `Bearer`. Header and scheme NAMES are kept — knowing which
credential was rejected is why the line is worth storing.

Five cases, including a guard rail that ordinary error text passes through
untouched: an over-broad redactor destroys the diagnostic value of every log
line, which is its own failure mode.

Unchanged limit: this removes CREDENTIALS, not PII.
…es both issuer spellings

Two low-severity findings from the second cross-vendor pass, both the same
mistake in different places: a check that answers 'is there a row' where the
caller needs 'would a real request resolve this'.

EXISTENCE CHECKS COUNTED DEAD STATES AS LIVE. `knownAgentIds` included
agents with `status: 'disabled'`, but dispatch resolves against the ACTIVE
registry. `knownKeyIds` included revoked keys, but authentication skips a
revoked record. So an operator could bind a revoked key to a disabled agent,
get a 201 with no warnings, and see a green row — the exact
dead-but-configured-looking state #571 added these checks to prevent, one
layer along. Both sets now filter to what would actually resolve.

CLIENT ROWS ARE KEYED BY ISSUER STRING, COMPARED BY `sameIssuer`. Two rules
over one value: the comparison treats `https://as.example` and
`https://as.example/` as the same identifier, the storage lookup does not.
A client stored under one spelling is invisible to a lookup using the other,
so rotation detection says 'same issuer' while `loadClient` says 'no
client' and the install is pushed into another acquisition flow, or simply
cannot refresh. `loadClient` now tries both spellings, exact first.

Deliberately NOT normalising the key outright: that would orphan every row
already written with a trailing slash. Trying both is backward compatible,
and an install that never hit this keeps its single lookup.
…he new guards

42 caught, 0 skipped.

Two of my own commits silently invalidated harness entries by editing the
source they match on — extracting `assertMaskingCrossed` changed the
assertion's indentation, and adding the wire-byte cap rewrote the body-cap
condition. Both showed as 'SKIPPED (mutation no longer applies)', which is
lost coverage on the endpoint's most important controls while still reading
like a normal line in the output.

Also adds entries for three guards that had none, all added by this run of
fixes: the wire-byte measurement specifically (distinct from the byte cap
generally, so dropping it fails the re-serializes-small case), the JSON-RPC
batch refusal, and the idempotency-replay exemption.

Worth knowing about this harness: it matches EXACT source strings, so any
refactor of a guarded line silently drops its mutation. It is run by hand —
not wired into CI — so nothing else notices. The wrapper I was driving it
with also ended on `echo`, which meant a non-zero harness exit was reported
as success; that is fixed on my side but the harness's own
`[ "$fail" -eq 0 ]` was always honest.
@Weegy Weegy changed the title MCP 2026-07-28 readiness: Waves 0-2 + adversarial review round (16 defects found and fixed) MCP 2026-07-28 readiness (waves 0-6) + #571 binding fix + two-pass security review Aug 7, 2026
@Weegy
Weegy marked this pull request as ready for review August 7, 2026 09:36
@Weegy
Weegy merged commit 3625b77 into main Aug 7, 2026
9 checks passed
Weegy added a commit that referenced this pull request Aug 7, 2026
Resolves five conflicts against main (#550 MCP waves 0-6, #624, #613):

- mcpClient.ts: keep BOTH the new pool-lifetime members (entries map,
  idleTtlMs, MCP_POOL_IDLE_TTL_MS, mcpPoolScopeMatches) and main's
  structuredSink / pendingInput options and outputSchemas cache.
- src/index.ts: keep the runtimeMcpManager handle alongside main's
  structured-sink wiring and the W2-1 (#544) input replayer.
- routes/agentBuilder.ts: keep main's W0-1 ownership check on
  DELETE /mcp-servers/:id/token (404/403 fail-closed) and invalidate the
  pooled connection after the token row is deleted.
- docs/adr: main landed 0007-mcp-client-id-metadata-documents (2026-07-30)
  first, so this ADR is renumbered 0007 -> 0008 and every reference updated.
- CHANGELOG: both Unreleased entries retained.

Also fixes the mcpPool fixture: serverRow() predates #550 and omitted the
required `delegation` field, so resolveMcpUserKey failed closed and the
token-revocation test got 403 instead of 204.

Verified on the merge result: lint, typecheck and the core-decoupling
ratchet pass, and `npm test` is green 3/3 (6075 pass, 0 fail, ~35-49s) --
the parallelism failures documented in the PR body are resolved by #613,
now on main. Both new behaviours mutation-checked: disabling
onMcpServerChanged turns 3 tests red, weakening the '#' pool-key separator
turns 2 red.
Weegy added a commit that referenced this pull request Aug 12, 2026
… size (#666)

#566 asks to split the two heaviest test files. Re-measured on 3c86f1c
first, and the premise no longer holds:

  slotTypecheckPipeline  18394 ms (issue) ->  2032 ms
  cliBackendDetector     16506 ms (issue) ->  3772 ms
  slowest file today: devplatform/dockerBackend.test.ts, 5163 ms

Neither named file is the slowest any more, and cliBackendDetector is 175
lines — splitting it into 4-6 files would be churn with nothing behind it.
So this ships what the issue is actually about: nothing detects a file
approaching the ceiling, which is why the margin went unwatched since #550
sized it.

`--test-timeout` kills the FILE, not the leaf, and blames whichever leaf was
running. The per-file total was not even observable: spec and tap flatten a
glob to suite names, so the filename is gone. `test:summary` is the one
event carrying both `file` and `duration_ms` — a second reporter on `npm
test` records it at no extra cost, and check-test-file-durations.mjs turns
it into a gate.

The gate is a FRACTION of the ceiling (warn 25%, fail 50%), not a committed
ms baseline. Durations vary by machine — this suite is ~36 s locally on 16
cores and ~172 s on a 4-vCPU runner — so an absolute number would be either
permanently red or permanently asleep, and would need re-committing as the
suite grows. That stale-baseline failure has bitten this repo before. The
ceiling is parsed out of the `test` script rather than duplicated, so
lowering --test-timeout tightens the guard automatically.

Today: 572 files, slowest 5163 ms, 23.2x headroom.

Mutation-checked rather than assumed — the guard exits 1 at 60000 ms (50%),
exits 0 at 59999 ms with a WARN, and also exits 1 on an empty file list or a
missing input, so it cannot pass while checking nothing.
Weegy added a commit that referenced this pull request Aug 13, 2026
The client half of MRTR shipped in PR #550: when a REMOTE MCP server answers a
`tools/call` with `resultType: "input_required"`, `McpManager` parks the call and
omadia renders an input card. Nothing in omadia's OWN MCP server path ever
produced that shape, so the public endpoint could only ever answer with a result
or an error. A tool that needed one more value from the human had two options,
both bad: fail with prose no machine can act on, or guess.

This is the missing direction. A dispatched tool signals "I need these fields"
in-band, and the endpoint renders it as MRTR so an ordinary MCP client can
collect the values and retry.

Why in-band rather than a third `ToolDispatchResult` variant: that type is shared
by every dispatch surface — chat, routines, sub-agents, this endpoint — and a new
variant would force all of them to grow a branch for a case only this endpoint
can render. So the signal rides the result string as a JSON sentinel
(`_pendingInputRequest`), the same convention `_pendingUserChoice` already uses
for plugin-emitted choice cards. A surface that does not understand it shows the
tool's own message and is no worse off than before.

Why the retry needs no server-side state: MRTR has the CLIENT retry the original
request with `inputResponses` added, so the arguments come back from the caller.
That is what keeps this working on a deliberately stateless endpoint — omadia
parks nothing, holds no correlation id, and any instance behind the load balancer
can serve the retry. The retry key is `inputResponses`, the SAME key
`REPLAY_ARG_KEY` uses on the client half, so both directions speak one
vocabulary. Field validation reuses `parseMcpInputRequests`, so a request omadia
SENDS and one it RECEIVES are clamped identically and neither direction is the
lenient one.

Three outcomes when the sentinel is present:
  - malformed request → an ordinary tool error naming the reason, rather than
    shipping our raw sentinel JSON to the caller as if it were an answer;
  - already answered  → an ordinary tool error, mirroring
    `MCP_INPUT_MAX_REPLAY_DEPTH` on the client half: one round trip, not a loop;
  - otherwise         → the MRTR body.

The MRTR body never carries `isError` — the client half's `isInputRequiredResult`
refuses to read an `isError` result as a card, so flagging it would make omadia's
own endpoint unreadable by omadia's own client. A dispatch that genuinely failed
is excluded for the same reason a failure has no pending continuation. `content`
is populated alongside `resultType` so a pre-MRTR client still shows the human
what is being asked instead of an empty result, and the #647 provenance `_meta`
rides the new body shape unchanged.

Documented in the endpoint README (a public API gaining a response shape without
documentation is a gap, not a detail).

Tests (`test/publicMcp/publicMcpInputRequired.test.ts`, 11): the pure module
(parse/bounce/render) plus the real mounted endpoint through `startHarness` —
the same `mountPublicMcp` production calls. The round trip is asserted whole:
ask → retry with `inputResponses` → the tool receives them verbatim and finishes.
A test that only proved the ASK would pass against a broken retry leg.

Mutation check, verified red: disabling the rendering kills 4 of the 6 endpoint
tests. The two that stay green are the ones asserting the feature must NOT fire
(a failed call, an ordinary result), which is the correct signature.

Verification: `test/publicMcp/*` 203/203 pass, `tsc --noEmit` clean, eslint clean,
`typecheck:test` ratchet 406 = baseline.

Depends on #570 (PR #676) for the flow to be observable end-to-end in a default
configuration — the client half is interned without it.

Closes #544
Weegy added a commit that referenced this pull request Aug 14, 2026
…bject (#691)

* fix(#568): bridge channel turns to per_user MCP tokens via the IdP subject

A channel turn (Teams/Telegram/Slack) keyed its MCP identity on the
KG-canonical omadia uuid, while `/mcp-servers/:id/authorize` stores a
`per_user` OAuth token under the session's `sub` — the auth provider's own
subject. Two namespaces that never met, so a per-user token granted in the
Admin UI was never found from a channel and every such call failed closed.

The bridge was genuinely missing: `resolveOrCreateChannelIdentity` returned
ids only, so nothing downstream could recover the subject.

- plugin-api: `ChannelIdentityIngest.authSubject` records WHICH IdP subject
  authenticated, and the resolve result gains `clusterAuthSubject`, read
  from any identity in the cluster. `authSubjectProps` lives here so both
  graph backends persist it under the same property names.
- neon + in-memory: persist the subject, backfill it on the fast path (every
  login re-enters there, so pre-existing identities are not stranded), and
  merge rather than replace, so a channel-side re-resolve cannot erase a
  subject a login established.
- `resolveTurnOwnerIdentity` now returns `{ omadiaUserId, authSubjectKey }`,
  carried by the single round-trip the turn already pays for.
- the two orchestrator producers prefer the subject and keep the canonical
  uuid as the fallback, so a channel-only user is unchanged.

Recorded as an explicit fact rather than inferred: for the local provider
`providerUserId` happens to be the lowercased email today, and reusing
`email` would be exactly the convention that rots silently in a credential
path.

Not a trust change — both values are KG-attested and neither is
client-controlled. Absence of a subject still means "no token to inherit",
never a substitute key, so W0-1's confused-deputy fix is preserved.

Known limit, deliberately not papered over: a cluster that merged a
local-password and an Entra login holds two subjects; the pick is
deterministic (most recently seen) and a token authorized under the other
one is not found, which fails closed. Widening that means teaching the
token lookup to try an ordered key set — a change to the credential read
path, out of scope here.

Rewrites two tests that asserted the mismatch as desired behaviour; they
were green only because they checked that SOME key was produced, never that
it was a key a token could exist under. Same shape as the #550 security
test that pinned the vulnerable variant.

Closes #568

* test(#568): execute the Neon bridge against real Postgres

`slice1bUserCluster.test.ts` covers this contract against the in-memory
graph plus a text-level assertion that the Neon SQL filters by tenant.
Neither executes the Neon implementation, and Neon is the production path.

The three things most likely to be wrong here are all SQL-shaped and
invisible to both:

- the `properties || $3::jsonb` backfill on the fast path — a `jsonb_set`
  of a missing key, or the operands the wrong way round, silently no-ops
  or clobbers,
- the sibling lookup crossing IS_IDENTITY_OF in the right direction,
- reading the subject INSIDE the caller's transaction, so it reflects the
  backfill the same call just wrote.

Adds a cross-tenant case too: a subject leaking across tenants would hand
a channel turn a key that authorizes as someone else entirely.

Mutation check: dropping `|| $3::jsonb` from the fast-path UPDATE and
rebuilding `dist/` turns the backfill test red (1 of 6); the other five
correctly survive. Restored, rebuilt, 6/6 green against pgvector/pg17.

Var chain includes GRAPH_PG_TEST_URL and MEMORY_PG_TEST_URL, both of which
CI sets, so this runs in CI rather than skipping into a green no-op. With
no Postgres it skips with a logged reason (issue #572).
@Weegy
Weegy deleted the feat/mcp-2026-07-28-wave0-wave1 branch August 14, 2026 06:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants